Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing statics from codebase #117

Merged
merged 17 commits into from
Apr 5, 2019
Merged

Removing statics from codebase #117

merged 17 commits into from
Apr 5, 2019

Conversation

badrishc
Copy link
Contributor

@badrishc badrishc commented Apr 3, 2019

We have used ThreadStatic until now, for storing thread-local variables. This does not work when you have multiple instances of FASTER operating at the same time. This PR fixes the problem by using instance-specific thread-local variables instead. We have implemented our own version of thread-local for this purpose, called FastThreadLocal. Incidentally, FastThreadLocal is optimized to be around 10x faster than ThreadLocal in our micro-benchmarks. There is a configurable (in code) limit of 128 instances of a particular thread-local variable type. This may be relaxed in future if the requirement comes up. With this PR, there are no shared objects between instances, except for the buffer pool which is shared static for the entire process.

Copy link
Contributor

@gunaprsd gunaprsd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FasterThreadLocal<T>.InitializeThread isn't thread-safe. It is being called in LightEpoch.Acquire and can lead to concurrency issues.

Copy link
Contributor

@gunaprsd gunaprsd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain how the current implementation of FastThreadLocal<T> is instance specific? I am confused because the values array is thread static which I suppose means that two objects of FastThreadLocal<int> access the same values array if done from the same thread.

@badrishc
Copy link
Contributor Author

badrishc commented Apr 4, 2019

FasterThreadLocal<T>.InitializeThread isn't thread-safe. It is being called in LightEpoch.Acquire and can lead to concurrency issues.

Fixed as follows:

            if (values == null)
            {
                Interlocked.CompareExchange(ref values, new T[kMaxInstances], null);
            }

Can you explain how the current implementation of FastThreadLocal<T> is instance specific? I am confused because the values array is thread static which I suppose means that two objects of FastThreadLocal<int> access the same values array if done from the same thread.

FastThreadLocal has a thread-static array, i.e., each thread sees its own copy of the array. Each instance of the object (regardless of threads) will correspond to a unique entry offset (id) in the array. You can visualize this as a 2-d matrix of threads and instances, with one array per thread.

@peterfreiling-zz
Copy link

FasterThreadLocal<T>.InitializeThread isn't thread-safe. It is being called in LightEpoch.Acquire and can lead to concurrency issues.

Fixed as follows:

            if (values == null)
            {
                Interlocked.CompareExchange(ref values, new T[kMaxInstances], null);
            }

Why is this necessary for a thread static variable? Won't each thread sees a different copy of "values", making the Interlocked operation unnecessary?

@badrishc
Copy link
Contributor Author

badrishc commented Apr 4, 2019

FasterThreadLocal<T>.InitializeThread isn't thread-safe. It is being called in LightEpoch.Acquire and can lead to concurrency issues.

Fixed as follows:

            if (values == null)
            {
                Interlocked.CompareExchange(ref values, new T[kMaxInstances], null);
            }

Why is this necessary for a thread static variable? Won't each thread sees a different copy of "values", making the Interlocked operation unnecessary?

Correct, it's not needed. My original code is fine, I think.

Copy link

@peterfreiling-zz peterfreiling-zz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

peterfreiling and others added 5 commits April 4, 2019 10:01
…l.values for threads that haven't initialized values yet
commit 2ba964526c96c5c6852d1b702348a5bc9fa94dcb
Merge: f2b2c1f 765c083
Author: Badrish Chandramouli <badrishc@microsoft.com>
Date:   Thu Apr 4 10:54:51 2019 -0700

    Merge branch 'nostatics' of https://github.com/Microsoft/FASTER into readobjfix

commit f2b2c1f84b0bac97da222de4f3c929743742ecf2
Author: Badrish Chandramouli <badrishc@microsoft.com>
Date:   Thu Apr 4 10:54:09 2019 -0700

    Fixes to addresses, cleanup object read logic

commit 765c083
Author: Peter Freiling <peterfr@microsoft.com>
Date:   Thu Apr 4 10:01:26 2019 -0700

    Fixing issue where LightEpoch.IsProtected dereferences FastThreadLocal.values for threads that haven't initialized values yet

commit b78aee3
Author: Peter Freiling <peterfr@microsoft.com>
Date:   Wed Apr 3 20:43:28 2019 -0700

    Removing extra buffer copy

commit cd0da5ec37469486e0b6cd978c2088a5ee6bb598
Author: Badrish Chandramouli <badrishc@microsoft.com>
Date:   Wed Apr 3 19:59:53 2019 -0700

    Updates

commit 8d2ceeb6afcdaf2dacb98c0fa905a66bba7f3ae9
Author: Badrish Chandramouli <badrishc@microsoft.com>
Date:   Wed Apr 3 19:22:39 2019 -0700

    Fix for reading objects
@badrishc badrishc merged commit 7517324 into master Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants